ADFA-5231: Refuse edits computed against a joined stale pin - #1746
Open
itsaky-adfa wants to merge 5 commits into
Open
ADFA-5231: Refuse edits computed against a joined stale pin#1746itsaky-adfa wants to merge 5 commits into
itsaky-adfa wants to merge 5 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
itsaky-adfa
changed the title
jatezzz
reviewed
Aug 26, 2026
jatezzz
reviewed
Aug 26, 2026
jatezzz
requested changes
Aug 26, 2026
itsaky-adfa
force-pushed
the
ADFA-5231-stale-pin-refusals
branch
from
August 26, 2026 15:33
8154d68 to
09d7a86
Compare
itsaky-adfa
force-pushed
the
ADFA-5231-stale-pin-refusals
branch
from
August 27, 2026 09:06
2705c6f to
e0f2563
Compare
jatezzz
reviewed
Aug 27, 2026
| // Resolving the file and taking the read lock can both block long enough for the user to | ||
| // type, and nothing between here and performCodeAction re-checks the insertion point. | ||
| logger.debug("dropping import candidates for {}: buffer moved while computing", nioPath) | ||
| return@withLiveKtFile emptyMap() |
Collaborator
There was a problem hiding this comment.
@itsaky-adfa This refusal reaches the user as a false statement.
postExec reads an empty map as "nothing importable" and flashes msg_no_imports_found - "No imports found" (AddImportAction.kt:140-142). But classifiers had to be non-empty to get this far: imports were found, then dropped because the buffer moved. Of the four refusal paths this PR adds, this is the only one that is not silent, and it tells the user the opposite of what happened.
msg_extract_method_file_changed - "The file changed. Try extracting again." - already exists for exactly this, and is the shared message your reply asked for. Distinguishing the two empty returns is a couple of lines, so worth doing here rather than in the follow-up.
The pin is process-wide, so a request arriving during another feature's scope joins it and gets that scope's text, however old. The action layer stamps its version guard from the live buffer, so a joined stale pin passes the guard and then applies offsets measured against older text to the newer buffer. Every site whose output is an edit now checks isStale and degrades. The repro test needed a competing acquisition to reproduce at all: bumping the document version only updates FileManager, and a second KtFile is installed by the index's own refresh. Without it both tests passed unpinned.
The variants carry raw PSI offsets and nothing downstream re-checks them against the document, so a joined stale pin inserted !!/? at the wrong offset. Its body moves into an internal computeNullSafetyVariants taking AbstractCompilationEnvironment, mirroring the three sibling actions, so the guard is reachable from a test. The completion offset is clamped to the pinned text's length: the staleness guard compares against the current document version, not the version params.position was measured against, and CompletionParams carries none.
The stale-pin refusal this replaces returned before analyzingVariant, so an INTERACTIVE request never reached the scheduler and stopped preempting the older completion whose pin it joined - leaving that older one to publish items for a caret the user had already moved past.
The pre-acquisition check only covers a pin that was already stale on acquisition. The wider window is the computation itself: nothing between these sites and performCodeAction re-checks the offsets the edits were measured against.
itsaky-adfa
force-pushed
the
ADFA-5231-stale-pin-refusals
branch
from
August 27, 2026 16:13
e0f2563 to
031bc8a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack 4 of 5 for ADFA-5231. Closes the one hole the pin itself opens.
The problem
Because a second request for a pinned path joins the existing pin, it gets that scope's text - which can be older than the buffer. Before the migration every site resolved the current document version, so text and request coordinates were always coherent. Now they can diverge, and the worst case is a silent wrong edit:
A usage search holds a pin at version N. The user types (N+1) and invokes extract-method.
ExtractMethodActionstampsdocumentVersionfrom the live buffer (N+1) while the planner readsfileTextfrom the joined pin (N). The guard atExtractMethodAction.kt:136compares the stamp against the live buffer, passes, and offsets computed against version-N text are applied to the version-N+1 buffer. The check that exists to prevent exactly this cannot see the mismatch.Completion had a crash variant:
originalTextfrom the older pin with an offset from the current request makes the placeholder splice throwIndexOutOfBoundsException.The fix
Every site whose output is an edit now checks
LiveKtFile.isStaleand refuses rather than computing against frozen text: both extraction planners, completion, organize-imports, implement-members, add-import, and the null-safety action. Each degrades to its existing "nothing to offer" answer. A refusal is recoverable; a wrong edit to the user's source is not.Navigation and info sites (
GoToDefinition,FindUsages,KotlinSignatureHelp) deliberately keep tolerating being one edit behind - they already document that, and their failure mode is a wrong jump rather than a corrupted file.GoToDefinition's comment claiming the caret offset and PSI come from the same text was false in the join case and is corrected.Also clamps the completion offset to the pinned text length, closing a narrower pre-existing crash where the request's position was measured against a snapshot that has since moved.
Trade-off you should weigh
Five features now silently do nothing while another scope holds a pin on the same path and the user has typed. That is the right trade against a wrong edit, but the real fix is shortening pin duration rather than degrading the victims - see the follow-ups on the ticket. The refusals are also currently indistinguishable from "nothing to do", which wants one shared "the file changed, try again" message.
Testing
461 tests, 0 failures.
StalePinEditRefusalTestcovers all six sites plus a control, and each guard is mutation-checked - forcingisStalefalse fails exactly the covering test. One test bumps the version with genuinely changed content, so it demonstrates the corruption these guards prevent rather than only that they fire.